change yroom class attribute to instance attribute and stop ystore in stop method #39
Conversation
jzhang20133
force-pushed
the
yroom-exception
branch
from
c1fe260 to
88d0cf7
Compare
pycrdt_websocket/yroom.py
Outdated
| self._update_send_stream, self._update_receive_stream = create_memory_object_stream( | ||
| max_buffer_size=65536 | ||
| ) |
There was a problem hiding this comment.
| self._update_send_stream, self._update_receive_stream = create_memory_object_stream( | |
| max_buffer_size=65536 | |
| ) |
jzhang20133
force-pushed
the
yroom-exception
branch
3 times, most recently
from
794b98d to
a993c27
Compare
jzhang20133
changed the title
jzhang20133
force-pushed
the
yroom-exception
branch
from
15e6901 to
a35810d
Compare
jzhang20133
force-pushed
the
yroom-exception
branch
2 times, most recently
from
29dc715 to
44b8f55
Compare
jzhang20133
force-pushed
the
yroom-exception
branch
from
d525b5e to
a803af3
Compare
| try: | ||
| await self.ystore.stop() | ||
| except RuntimeError: | ||
| pass |
There was a problem hiding this comment.
I'm not sure we should automatically stop the YStore when the YRoom is stopped.
WebsocketServer has an auto_clean_room parameter, maybe there should be an auto_stop_store as well, that we would use to do so if set to True.
Also, we should have YStore use the same exception handler pattern that we used for YRoom and WebsocketServer, and not catch exceptions here.
There was a problem hiding this comment.
I think it is a good idea to make it configurable. And also +1 for adding exception handler pattern in other logic to protect task group. I will address those.
In this stop method case, we are trying to except a RuntimeError that is thrown if "YStore not running" in cases like ystore is already stopped or ystore is not started yet but room crashed. I can create a specific exception type for this case.
2 participants
Previously, some same named instance attributes are created or defined outside of initializer and YRoom does not need class attributes hence we change yroom class attributes to instance attribute just to improve readability. In this PR we also stop ystore in yroom.stop() method.
Added a unit test to ystore stop inside yroom.stop() and unit test need #42 to be merged. Need to wait until #42 is merged and then rebased.